Skip to content

fix(otelaws): don't record 304 Not Modified as a span error - #9474

Open
pujitha24 wants to merge 4 commits into
open-telemetry:mainfrom
pujitha24:auto/issue-6331
Open

fix(otelaws): don't record 304 Not Modified as a span error#9474
pujitha24 wants to merge 4 commits into
open-telemetry:mainfrom
pujitha24:auto/issue-6331

Conversation

@pujitha24

@pujitha24 pujitha24 commented Aug 14, 2026

Copy link
Copy Markdown
Contributor

Motivation:
otelaws marks a span as an error and sets error.type whenever the AWS
SDK returns a non-nil error for an API call, including HTTP 304 Not
Modified responses. A 304 is returned by services that support
conditional requests (e.g. S3 GetObject with IfModifiedSince) when the
precondition is not satisfied, which is expected client behavior, not
a failure. The span already carries http.response.status_code, so
marking it as an error adds noise without new information.

Approach:
Add isNotModifiedResponseError, which unwraps the returned error with
errors.As into a *smithyhttp.ResponseError and checks whether its
HTTP status code is 304. initializeMiddlewareAfter now skips setting
error.type and span status Error only for this specific case; the
error returned to the caller is unchanged. Other 3xx codes (e.g. a 301
from a misconfigured bucket region) are intentionally left recorded
as errors, since those can indicate a real problem.

Validation:
go test ./... in
instrumentation/github.com/aws/aws-sdk-go-v2/otelaws, including a new
table-driven case that reproduces the issue: a route53 call returning
HTTP 304 no longer sets span status Error or error.type, while a
sibling case for HTTP 301 confirms other redirects are still recorded
as errors. Also ran golangci-lint run --allow-serial-runners, go vet
./..., and gofmt -l . (clean) in the same package.

Report: #6331
Signed-off-by: Pujitha Paladugu 10557236+pujitha24@users.noreply.github.com

Fixes #6331

@pujitha24
pujitha24 requested a review from a team as a code owner August 14, 2026 06:34
@codecov

codecov Bot commented Aug 14, 2026 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 84.5%. Comparing base (b0f0785) to head (d35c6de).

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##            main   #9474     +/-   ##
=======================================
- Coverage   84.5%   84.5%   -0.1%     
=======================================
  Files        203     203             
  Lines      16805   16813      +8     
=======================================
- Hits       14215   14212      -3     
- Misses      2110    2121     +11     
  Partials     480     480
Files with missing lines Coverage Δ
...tation/github.com/aws/aws-sdk-go-v2/otelaws/aws.go 97.7% <100.0%> (+0.2%) ⬆️

... and 1 file with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

ps-mir
ps-mir reviewed Aug 14, 2026
View reviewed changes

@ps-mir ps-mir left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A comment regarding refactoring. Spec wise, change seems ok, as instrumentation can have specific error handling.

Comment thread instrumentation/github.com/aws/aws-sdk-go-v2/otelaws/aws.go Outdated
// met, not a failure of the request, and should not be recorded as a span
// error. Other 3xx status codes (e.g. a 301 signaling a misconfigured
// bucket region) can indicate a real problem and are still recorded.
func isNotModifiedResponseError(err error) bool {

@ps-mir ps-mir Aug 14, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is packing two independent questions

  1. Can status code be extracted from the error (Does a response exist)
  2. Is that status code the one of interest (304)

Keeping it as functional is useful for testing, would recommend something like this

func responseStatusCode(err error) (code int, ok bool) {
        var respErr *smithyhttp.ResponseError
        if !errors.As(err, &respErr) {
                return 0, false
        }
        return respErr.HTTPStatusCode(), true
}

func isNotModifiedStatus(err error) bool {
        code, ok := responseStatusCode(err)
        return ok && code == http.StatusNotModified
}

ps-mir
ps-mir reviewed Aug 14, 2026
View reviewed changes
Comment thread CHANGELOG.md Outdated

### Fixed

- Don't mark spans as errors in `go.opentelemetry.io/contrib/instrumentation/github.com/aws/aws-sdk-go-v2/otelaws` when the AWS SDK returns an error wrapping an HTTP `304 Not Modified` response, such as from a conditional S3 `GetObject` request whose precondition (e.g. `IfModifiedSince`) was not satisfied.

@ps-mir ps-mir Aug 14, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need to be moved to bottom. Also please attach the PR number.

@pujitha24

pujitha24 commented Aug 14, 2026

Copy link
Copy Markdown
Contributor Author

Split isNotModifiedResponseError into responseStatusCode and isNotModifiedStatus as suggested, and added a direct test for responseStatusCode alongside the existing 304/301 cases. Also moved the changelog entry to the bottom of ### Fixed and attached (#9474).

ps-mir
ps-mir reviewed Aug 14, 2026
View reviewed changes

@ps-mir ps-mir left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Test for responseStatusCode can be dropped as its covered by tests of calling function. Rest looks ok.

Comment thread instrumentation/github.com/aws/aws-sdk-go-v2/otelaws/aws_test.go Outdated
assert.NotContains(t, input.Header[key], value)
}

func Test_responseStatusCode(t *testing.T) {

@ps-mir ps-mir Aug 14, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can dropped as Test_isNotModifiedStatus provides equivalent coverage for responseStatusCode.

@pujitha24
fix(otelaws): don't record 304 Not Modified as a span error
fb08330 
Motivation:
otelaws marks a span as an error and sets error.type whenever the AWS
SDK returns a non-nil error for an API call, including HTTP 304 Not
Modified responses. A 304 is returned by services that support
conditional requests (e.g. S3 GetObject with IfModifiedSince) when the
precondition is not satisfied, which is expected client behavior, not
a failure. The span already carries http.response.status_code, so
marking it as an error adds noise without new information.

Approach:
Add isNotModifiedResponseError, which unwraps the returned error with
errors.As into a *smithyhttp.ResponseError and checks whether its
HTTP status code is 304. initializeMiddlewareAfter now skips setting
error.type and span status Error only for this specific case; the
error returned to the caller is unchanged. Other 3xx codes (e.g. a 301
from a misconfigured bucket region) are intentionally left recorded
as errors, since those can indicate a real problem.

Validation:
go test ./... in
instrumentation/github.com/aws/aws-sdk-go-v2/otelaws, including a new
table-driven case that reproduces the issue: a route53 call returning
HTTP 304 no longer sets span status Error or error.type, while a
sibling case for HTTP 301 confirms other redirects are still recorded
as errors. Also ran golangci-lint run --allow-serial-runners, go vet
./..., and gofmt -l . (clean) in the same package.

Report: open-telemetry#6331
Signed-off-by: Pujitha Paladugu <10557236+pujitha24@users.noreply.github.com>
@pujitha24
test(otelaws): add direct unit test for isNotModifiedResponseError
f315695 
Covers the branch where errors.As fails to match a
*smithyhttp.ResponseError, which the existing table-driven
integration test never exercised, per codecov/patch feedback on
patch coverage.

Signed-off-by: Pujitha Paladugu <10557236+pujitha24@users.noreply.github.com>
@pujitha24
fix(otelaws): address review feedback on 304 helper and changelog
38e876a 
Split isNotModifiedResponseError into responseStatusCode (extracts the
HTTP status code from a wrapped smithyhttp.ResponseError) and
isNotModifiedStatus (checks for 304), per review, and move the
CHANGELOG entry to the bottom of the Fixed section with the PR number
attached.

Signed-off-by: Pujitha Paladugu <10557236+pujitha24@users.noreply.github.com>
@pujitha24
test(otelaws): drop redundant Test_responseStatusCode
d35c6de 
Test_isNotModifiedStatus already exercises responseStatusCode's
nil, non-response, and response-error branches, per review.

Signed-off-by: Pujitha Paladugu <10557236+pujitha24@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer
@ps-mir ps-mir ps-mir approved these changes
Reviewers whose approvals may not affect merge requirements

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

otelaws s3 error responses should not always be recorded

2 participants

@pujitha24 @ps-mir